feat(plugin): add crd-review command to check CRDs against Kubernetes and OpenShift API conventions #27
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
perdasilva
changed the title
perdasilva
force-pushed
the
openshift-api-conventions-review
branch
from
d62141f to
6ec6772
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
perdasilva
changed the title
perdasilva
force-pushed
the
openshift-api-conventions-review
branch
from
6ec6772 to
c0559cf
Compare
perdasilva
changed the title
perdasilva
changed the title
perdasilva
force-pushed
the
openshift-api-conventions-review
branch
from
c0559cf to
78c2d70
Compare
|
@perdasilva Can you provide more detail on how you generated this new command? Was it a case of giving claude the API conventions doc and this is what it came up with? I suspect having one that focused on upstream conventions would likely pick up more useful feedback, as our downstream conventions extend those. Perhaps we can see what claude comes up with for upstream conventions? |
perdasilva
force-pushed
the
openshift-api-conventions-review
branch
from
78c2d70 to
9048a12
Compare
@JoelSpeed Here's the prompt I used: I'll also add it to the PR description and keep it up-to-date as we iterate. I'll update it to include both upstream and downstream conventions. |
perdasilva
force-pushed
the
openshift-api-conventions-review
branch
from
9048a12 to
b8a57e9
Compare
|
@JoelSpeed I've updated it ^^ I also had to rework the configuration - more to plugin.json and add the openshift plugin to the marketplace.json list |
perdasilva
changed the title
perdasilva
force-pushed
the
openshift-api-conventions-review
branch
from
b8a57e9 to
f92c549
Compare
|
|
||
| #### API Structure | ||
| - **Required Fields**: Every object must have `kind` and `apiVersion` | ||
| - **Metadata**: Should include `namespace`, `name`, `uid`, optionally `resourceVersion`, `labels`, `annotations` |
There was a problem hiding this comment.
CRDs themselves are cluster-scoped right? So they can't set metadata.namespace? Flagging it as required will create false positives for every CRD the command inspects won't it?
| The command performs the following analysis workflow: | ||
|
|
||
| 1. **Repository Discovery** | ||
| - Scan repository for CRD files (YAML manifests in `config/`, `deploy/`, `manifests/` directories) |
There was a problem hiding this comment.
This Implementation section promises concrete analysis (discover CRDs, validate conventions, assign severities), but it never gives the agent actionable steps or heuristics to execute. Without instructions on how to locate CRD YAML, walk spec.versions[].schema, inspect Go types, or derive severities, the command will just restate guidelines instead of performing a real review. I think you should flesh this section out with the actual workflow (e.g., how to enumerate CRD files, what to look for when checking naming/spec/status, how to collect file & line references) so the agent can deliver the promised analysis
There was a problem hiding this comment.
ugh - isn't the whole point of this thing that it does what it's told? XD
There was a problem hiding this comment.
It did a pretty good job of locating them with just the project's root...let me see what I can do here to spruce this up
There was a problem hiding this comment.
yeah, I'm not sure. I've been testing it as it is and I've found that it locates the relevant files and seems to surface reasonable analysis and detect errors. I've tried a few things, naming convention violations, removing metav1.ObjectMeta, changing a discriminating union, etc. It even surfaced some interesting observations:
Finding:
// +kubebuilder:default:=0
// +kubebuilder:validation:minimum:=-2147483648
// +kubebuilder:validation:maximum:=2147483647
// +optional
Priority int32 `json:"priority"`
OpenShift Convention: While Kubernetes generally recommends int32, the documentation explicitly states using minimum and
maximum validation that matches int32 range. This is correct, but consider whether negative priorities are truly needed.
Recommendation: If negative priorities are not actually used in practice, consider restricting to
+kubebuilder:validation:minimum:=0 for clearer semantics. If negative priorities are intentional (for "lower than default"),
this is acceptable but should be well-documented in user-facing docs.
Severity: 💡 Info
Maybe I'll update it a bit to only focus on the Go types so it consumes a bit less of context...
|
Are you expecting the agent to look at Go API types as well? If so, could you spell out how to match a CRD version to the corresponding Go structs (directory patterns, filename hints, etc.) so the review can reliably connect the dots? |
|
@bentito its doing a pretty good job at finding and identifying the files and performing the analysis. Here's an example of it running against the OLMv0 repo: It's not perfect, but it gives some good and actionable feedback (also with line numbers). Maybe it's good enough for a first cut? |
It seems great! Yeah I think good enough for first cut. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bentito, perdasilva The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
Reopening to trigger lint |
perdasilva
force-pushed
the
openshift-api-conventions-review
branch
from
f92c549 to
34a62ad
Compare
|
The output you've posted is pretty good. You're right, not perfect, but has picked up on a lot of things. My concern here at the moment is that it's providing very high level and general feedback, rather than specific actionable items (e.g. you have documented it, go document everything) and that it's looking at already merged APIs. Generally we can't go back and fix APIs that have already merged without breaking people. In the openshift/api repo we are working on something equivalent that looks specifically at changes introduced within a branch/PR, to provide feedback before merge. Perhaps it's best to align on that? Or do you see a separate use case for having one that looks are entire API groups? |
I'm not terribly concerned about the fact that its looking at already merged APIs. Ultimately, the output of any of these tools should be thoroughly read and understood to be valid before any action is taken. Also, already merged doesn't mean released and it doesn't mean there's nothing to learn. I could add something to call out when something might be a breaking API change and caution the user? At the same time, I don't see the point in have two of these tools...should I just close this then? Here's it updated with breaking changes being called out: |
…API conventions Signed-off-by: Per Goncalves da Silva <[email protected]> Signed-off-by: Per G. da Silva <[email protected]>
perdasilva
force-pushed
the
openshift-api-conventions-review
branch
from
34a62ad to
6e3a7b0
Compare
I agree, though, reducing the time it takes to review the output should be a goal. If we throw away 90% of it's output after review, it's not effective in speeding us up
This depends. You can be more flexible in changes to v1alpha1 APIs, but, some of the suggestions it has output include changing the serialization of fields. You CANNOT change the serialization of a field once it has merged, even if you claim it isn't released yet. You cannot control which commit in your repo that clients vendor from and it won't always be the latest commit you think is sensible.
This would be good. In particular if there's a way it can work out if serialization is affected that would be good. An example of this would be changing a bool to a string |
It depends. On the OLMv1 side, we have experimental CRDs that are subject to change without notice. Similar to TP... |
I've added a change to get it to call out breaking changes, it called it out in a suggestion to move from bool to enum/string: |
While I agree with this statement, I don't know that's what is happening rn. There is a concise summary at the end, it seems to be picking up on valid issues, and when elucidating any of those points a bit of verbosity might be a good thing (explanations, references, examples, etc.). Besides, you can still ask claude to focus on specific areas of the report, a specific api, etc. If you factor in how long it takes to get a review, or waiting until the Tuesday meeting to talk to you guys, I think the speed up is pretty substantial. The report can be long especially if looking at multiple CRDs. Though, if you want to focus on just one you can call the tool with something like: So what do you think would be an improvement here? Maybe we could add another parameter to set some level of criticality or something like that? |
4 participants
What this PR does / why we need it:
crd-reviewcommand to the openshift plug-in. The command was generated using the following prompt:The crd-review takes a repository path as input and checks the CRDs therein for compliance with the OpenShift API conventions as defined here.
Example usage:
Special notes for your reviewer:
This command was generated by ClaudeCode
Checklist: